Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xarray.core.variable.as_variable part of the public API #1422

Merged
merged 8 commits into from
Jun 2, 2017

Conversation

benbovy
Copy link
Member

@benbovy benbovy commented May 23, 2017

Make xarray.core.variable.as_variable part of the public API and accessible as a top-level function: xarray.as_variable.

I changed the docstrings to follow the numpydoc format more closely.

I also removed the copy=False keyword arguments as apparently it was unused.

@fmaussion
Copy link
Member

Many tests fail because they expect to have a copy= kwarg somehow...

@benbovy
Copy link
Member Author

benbovy commented May 23, 2017

Oh yes I haven't run the tests locally before submitting this, I didn't expected that.

Mmm I don't see any current use of the copy keyword argument inside the as_variable function. Here is the only place where I found copy to be explicitly provided. Can I safely remove it also there? If I remove it the tests pass again (although I got 3 remaining failing tests that don't seem to be related to this issue as they fail either with or without the copy argument).

@shoyer
Copy link
Member

shoyer commented May 23, 2017

Mmm I don't see any current use of the copy keyword argument inside the as_variable function. Here is the only place where I found copy to be explicitly provided. Can I safely remove it also there?

Yes, we can safely remove the copy argument here. At point point, as_variable did not always return a new Variable object, but as you can see it always does that now.

Copy link
Member

@shoyer shoyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few minor suggestions, but in general I like this change!

----------
obj : object
Object to convert into a Variable.
If the object is already a Variable, return a shallow copy.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep these as bullet points for sphinx? I think that works for rendered docstrings if you're careful (e.g., see the docstring for xarray.Dataset.__init__).

If all else fails, attempt to convert the object into a Variable by
unpacking it into the arguments for creating a new Variable.
name : str, optional
Dimension name (only if data in `obj` is 1-dimensional).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say:

If provided:

  • obj can be a 1D array, which is assumed to label coordinate values along a dimension of this given name.
  • Variables with name matching one of their dimensions are converted into IndexVariable objects.

doc/api.rst Outdated
@@ -22,6 +22,7 @@ Top-level functions
full_like
zeros_like
ones_like
as_variable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move this to the "Advanced API" section at the bottom, next to Variable and IndexVariable.

- If all else fails, attempt to convert the object into a Variable by
unpacking it into the arguments for creating a new Variable.
name : str, optional
If provided
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a final colon :

@shoyer
Copy link
Member

shoyer commented May 24, 2017

We have some tests for as_variable, but it looks a little thin for public API:

def test_as_variable(self):

The remaining test failures should be fixed by #1423

Also allow passing to `as_variable` objects that have a `data`
attribute but no `values` attribute.
@benbovy
Copy link
Member Author

benbovy commented May 24, 2017

I've tried adding tests for lines in as_variable that were not yet covered.

@shoyer shoyer merged commit b877193 into pydata:master Jun 2, 2017
@shoyer
Copy link
Member

shoyer commented Jun 2, 2017

@benbovy thanks!

@benbovy benbovy deleted the as_variable_public_api branch June 10, 2017 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

xarray.core.variable.as_variable() part of the public API?
3 participants